Add support for creation of associated conversations with message via params #1301

begedin · 2017-12-15T14:00:05Z

What's in this PR?

Update

Still need:

tests for DataToAttributes plug
tests for Messages.create to check it's possible to create a Conversation alongside
tests for controller create action, to serve as integration between DataToAttributes and Messages.create

Original text

This PR progress on and hopefully closes #1293

It's been marked as discussion, but due to requiring the set of features described there for client PR code-corps/code-corps-ember#1597, I was forced to start on some type of implementation.

Will comment on specific changes, but to put it in short

ember-data, by default, does not support including records during post. It has a basic level of support for associating with existing has-many/many-to-many children in the form of the basic {type, id} identifier map, but not a full include, with all the child record's attributes.

ja_serializer does not support it either, so I was forced to provide my own implementation on both ends.

The alternatives are

create conversation manually from client, after creating message - To me, this may actually make sense. It makes the API less opinionated and allows potential other clients later on more flexibility on how they implement their own flow. On the downside, there's no clean way to clean up a message without child conversations if the process is interrupted.
Let the API infer which child records to create - This makes the API extremely opinionated, forces the client down an inflexible route and creates coupling between the two, so I'm against it. We would have to agree on some non-standard way to provide a list of user ids to create conversations for. I guess, we could define a message has many users through conversations relationship and do what we do with project skills, project categories, etc, but really, which is one way where I could agree to do it that way
Can't think of any other approach at the moment

References

Progress on: #1293
Tied to: code-corps/code-corps-ember#1597

begedin · 2017-12-15T14:02:20Z

lib/code_corps/messages/conversations.ex

+    |> Changeset.cast(attrs, [:user_id])
+    |> Changeset.validate_required([:user_id])
+    |> Changeset.assoc_constraint(:user)
+  end


This changeset is needed to cast_assoc. The message_id is provided automatically by cast_assoc and is not part of the params.

This will need a test.

begedin · 2017-12-15T14:03:23Z

lib/code_corps/messages/messages.ex

+    |> Changeset.validate_required([:author_id, :project_id])
+    |> Changeset.assoc_constraint(:author)
+    |> Changeset.assoc_constraint(:project)
+    |> Changeset.cast_assoc(:conversations, with: &Messages.Conversations.create_changeset/2)


create requires more work than the default changeset does. We should probably add more tests for the create function to make sure these changes work. I don't think there's an explicit need to extract a create changeset, though.

There's an argument to be made here that this belongs in its own changeset, akin to Messages.Conversations.create_changeset.

@begedin this is the nested POST, right? I remember doing this once and I had to create a changeset.

I think what I did was take the nested ids, pass it through |> Enum.map(&Changeset.change/1) and then put_assoc(changeset, :conversations, conversation_changesets) or something like that. But this looks much better if I am understanding it right.

But that required the nested ids to already exist in the DB. Not sure what the case is here.

@snewcomer The ids are not in the database in this case. The child record is created alongside the parent.

begedin · 2017-12-15T14:08:37Z

lib/code_corps_web/plugs/data_to_attributes.ex

+        false -> parsed |> Map.put(type, attributes)
+      end
+    end)
+  end


The critical part of the included support is here.

I approached this by following the existing doc for this plug, where it's stated that the goal of the plug is to convert JSON API params into a map of params suitable for Ecto.Changeset

From that, the goal was

Included belongs_to records are merged with params as a submap property, with the key identical to the belongs to record type

Included has_many records are merged with params as a list property, with the key being a pluralized version of the record type

This allows us to simply call cast_assoc(changeset, :record_type, opts) and the casting works out of the box.

I did not work on inferring the pluralized type directly and instead opted to explicitly provide these when calling the plug. It should be sufficient for our needs, but there is a way to infer instead.

Since the plug has become quite complex, I think we should add tests for it.

begedin · 2017-12-15T14:20:25Z

I've fixed existing tests.

We still need:

tests for DataToAttributes plug
tests for Messages.create to check it's possible to create a Conversation alongside
tests for controller create action, to serve as integration between DataToAttributes and Messages.create

joshsmith · 2017-12-15T18:34:47Z

lib/code_corps/messages/messages.ex

+    |> Changeset.validate_required([:author_id, :project_id])
+    |> Changeset.assoc_constraint(:author)
+    |> Changeset.assoc_constraint(:project)
+    |> Changeset.cast_assoc(:conversations, with: &Messages.Conversations.create_changeset/2)


There's an argument to be made here that this belongs in its own changeset, akin to Messages.Conversations.create_changeset.

joshsmith · 2017-12-15T18:39:44Z

lib/code_corps_web/plugs/data_to_attributes.ex

+            records ++ [record]
+          end)
+        false -> parsed |> Map.put(type, attributes)
+      end


Is there any reason to do a case here over an if/else?

joshsmith · 2017-12-15T18:41:02Z

lib/code_corps/messages/conversations.ex

+    |> Changeset.cast(attrs, [:user_id])
+    |> Changeset.validate_required([:user_id])
+    |> Changeset.assoc_constraint(:user)
+  end


This will need a test.

snewcomer

Don't have the full context of what is going on here. Do the conversations already exist in the DB when posting the message?

snewcomer · 2017-12-15T18:50:39Z

lib/code_corps/messages/messages.ex

+    |> Changeset.validate_required([:author_id, :project_id])
+    |> Changeset.assoc_constraint(:author)
+    |> Changeset.assoc_constraint(:project)
+    |> Changeset.cast_assoc(:conversations, with: &Messages.Conversations.create_changeset/2)


@begedin this is the nested POST, right? I remember doing this once and I had to create a changeset.

I think what I did was take the nested ids, pass it through |> Enum.map(&Changeset.change/1) and then put_assoc(changeset, :conversations, conversation_changesets) or something like that. But this looks much better if I am understanding it right.

snewcomer · 2017-12-15T18:51:00Z

lib/code_corps/messages/messages.ex

+    |> Changeset.validate_required([:author_id, :project_id])
+    |> Changeset.assoc_constraint(:author)
+    |> Changeset.assoc_constraint(:project)
+    |> Changeset.cast_assoc(:conversations, with: &Messages.Conversations.create_changeset/2)


But that required the nested ids to already exist in the DB. Not sure what the case is here.

joshsmith · 2017-12-15T18:57:45Z

@snewcomer the hierarchy is like:

Message
└── Conversation
    └── ConversationPart
    └── ConversationPart

A message could be written with multiple recipients in mind: think of a broadcast message as an example. If we wanted to send an update to all the volunteers, it would be done by creating a message that specifies multiple recipients, all of whom have a conversation created individually for the message.

But a message could also be 1:1, for example individually messaging a user, or a user individually messaging a project. In this case there would only be one conversation.

snewcomer · 2017-12-15T19:15:17Z

@joshsmith gotcha. On the frontend, the conversation-part is not serialized up though, correct? At one point does the conversation-part get created? Does that happen completely separate from creating a message?

joshsmith · 2017-12-15T21:20:18Z

@snewcomer yeah, the first conversation part is not the first line of the conversation, which I know is confusing, but necessary due to the 1:n nature of messages. It's similar to a comment thread of a GitHub issue. The initial part of the thread is actually the issue body, while everything else is specifically a comment.

joshsmith · 2017-12-16T01:35:03Z

lib/code_corps_web/plugs/data_to_attributes.ex

+        end)
+      else
+        parsed |> Map.put(type, attributes)
+      end


I had to fix this fn because Map.update's fun parameter only takes a single value (in this case the accumulated records) and we needed to provide the cast_assoc with an array with the default argument if the key doesn't yet exist (i.e. [attributes]).

I think this is the logic you were looking for @begedin. Spent quite some time debugging and trying to figure this out. More than I'm happy to admit.

Actually, the idea is, if there is a many_mapping key, then the included record is for a has_many relationship, so it needs to be put into a list.

If there is no many_mapping key, then the included record is for a belongs_to relationship, so it needs to be a map.

We don't cast a belongs to this way anywhere yet, so it doesn't matter for now, but that was the intended approach. Had I had the chance to add a test yesterday, this would have been clearer.

Actually, you did the exact thing that was needed. Sorry, this is difficult to follow

begedin · 2017-12-16T10:27:52Z

Instead of unit testing each individual bit, I opted to integration test on Messages.create/1 and the controller action. The individual bits (changesets) aren't that complex and their behavior is covered with these integration tests.

I think this ought to be good to go for now.

joshsmith · 2017-12-17T10:13:12Z

This will need tests fixed for the has_many added to conversations.

joshsmith · 2017-12-18T04:54:06Z

config/dev.exs

@@ -43,7 +43,7 @@ config :code_corps, CodeCorps.Repo,
  pool_size: 10

 # CORS allowed origins
-config :code_corps, allowed_origins: ["http://localhost:4200"]
+config :code_corps, allowed_origins: ["http://localhost:4200", "chrome-extension://pfdhoblngboilpfeibdedpjgfnlcodoo"]


This needs removed.

joshsmith · 2017-12-18T05:24:10Z

Will need tests for channels now.

Also, I think we should be propagating new conversation parts to the conversation.updatedAt so we can order conversations by their last updated time rather than their created time. The UI will then prefer the updated time.

begedin · 2017-12-18T06:40:27Z

I'll add the required tests today, but I'd say we're getting to a point where i'd consider the API side mergeable. It does not kill existing features, so there's little reason for holding off and it would make our life easier.

joshsmith

Some questions here.

joshsmith · 2017-12-18T20:14:15Z

lib/code_corps_web/channels/conversation_channel.ex

    end
  end
-  def join(_, _, _) do
-    {:error, %{reason: "unauthenticated"}}
-  end


If the Repo.get returns nil then we return {:error, %{reason: "unauthenticated"}}. I think this changes the behavior we had wanted originally, which is determining whether the user is authenticated or not, hence the pattern match on assigns.

joshsmith · 2017-12-18T20:14:48Z

test/lib/code_corps/messages/messages_test.exs

+      {:ok, %ConversationPart{id: id}} = Messages.add_part(attrs)
+      assert_broadcast("new:conversation-part", %{id: ^id})
+      CodeCorpsWeb.Endpoint.unsubscribe("conversation:#{conversation.id}")
+    end


joshsmith · 2017-12-18T20:16:52Z

test/support/channel_case.ex

-      import Ecto
-      import Ecto.Changeset
+      import CodeCorps.Factories
+      import CodeCorpsWeb.ChannelCase


Is there a reason this module is being imported into its own using? I'm guessing it's because it's importing its own namespace into the tests, but is that necessary if the case is used?

It's the general pattern for other XCase modules. Allows us to declare helper methods within this module's namespace, which the test cases can then use.

- Fix DataToAttributes plug's parse_included fn - Add conversation parts to view and controller - Add conversation channel

begedin mentioned this pull request Dec 15, 2017

Conversations Feature code-corps/code-corps-ember#1597

Merged

14 tasks

begedin commented Dec 15, 2017

View reviewed changes

begedin force-pushed the 1293-accomodating-messages-api-to-match-client branch 2 times, most recently from 3d05903 to 6054806 Compare December 15, 2017 14:04

begedin commented Dec 15, 2017

View reviewed changes

joshsmith reviewed Dec 15, 2017

View reviewed changes

snewcomer reviewed Dec 15, 2017

View reviewed changes

joshsmith reviewed Dec 16, 2017

View reviewed changes

joshsmith added the in progress label Dec 16, 2017

joshsmith assigned begedin Dec 16, 2017

joshsmith changed the title ~~Added support for creation of associated conversations with message via params~~ Add support for creation of associated conversations with message via params Dec 16, 2017

joshsmith reviewed Dec 18, 2017

View reviewed changes

begedin mentioned this pull request Dec 18, 2017

Modify Plugs.DataToAttributes to follow JSON API sideposting draft #1303

Open

joshsmith requested changes Dec 18, 2017

View reviewed changes

Add support for creating associated conversations with message

7546651

- Fix DataToAttributes plug's parse_included fn - Add conversation parts to view and controller - Add conversation channel

joshsmith force-pushed the 1293-accomodating-messages-api-to-match-client branch from 2c31b58 to 7546651 Compare December 18, 2017 22:52

joshsmith merged commit 8d55213 into develop Dec 18, 2017

joshsmith deleted the 1293-accomodating-messages-api-to-match-client branch December 18, 2017 22:58

joshsmith mentioned this pull request Dec 18, 2017

Implement realtime updates of messages with client via channels #1284

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for creation of associated conversations with message via params #1301

Add support for creation of associated conversations with message via params #1301

begedin commented Dec 15, 2017 •

edited

Loading

begedin Dec 15, 2017

joshsmith Dec 15, 2017

begedin Dec 15, 2017

joshsmith Dec 15, 2017

snewcomer Dec 15, 2017 •

edited

Loading

snewcomer Dec 15, 2017

begedin Dec 16, 2017

begedin Dec 15, 2017

begedin Dec 15, 2017

begedin commented Dec 15, 2017 •

edited by joshsmith

Loading

joshsmith Dec 15, 2017

joshsmith Dec 15, 2017

joshsmith Dec 15, 2017

snewcomer left a comment

snewcomer Dec 15, 2017 •

edited

Loading

snewcomer Dec 15, 2017

joshsmith commented Dec 15, 2017 •

edited

Loading

snewcomer commented Dec 15, 2017

joshsmith commented Dec 15, 2017

joshsmith Dec 16, 2017

begedin Dec 16, 2017

begedin Dec 16, 2017

begedin commented Dec 16, 2017

joshsmith commented Dec 17, 2017

joshsmith Dec 18, 2017

joshsmith commented Dec 18, 2017

begedin commented Dec 18, 2017

joshsmith left a comment

joshsmith Dec 18, 2017

joshsmith Dec 18, 2017

joshsmith Dec 18, 2017

begedin Dec 20, 2017

Add support for creation of associated conversations with message via params #1301

Add support for creation of associated conversations with message via params #1301

Conversation

begedin commented Dec 15, 2017 • edited Loading

What's in this PR?

References

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begedin commented Dec 15, 2017 • edited by joshsmith Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer left a comment

Choose a reason for hiding this comment

snewcomer Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshsmith commented Dec 15, 2017 • edited Loading

snewcomer commented Dec 15, 2017

joshsmith commented Dec 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begedin commented Dec 16, 2017

joshsmith commented Dec 17, 2017

Choose a reason for hiding this comment

joshsmith commented Dec 18, 2017

begedin commented Dec 18, 2017

joshsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begedin commented Dec 15, 2017 •

edited

Loading

snewcomer Dec 15, 2017 •

edited

Loading

begedin commented Dec 15, 2017 •

edited by joshsmith

Loading

snewcomer Dec 15, 2017 •

edited

Loading

joshsmith commented Dec 15, 2017 •

edited

Loading